Skip to content

feat: add flag for relaxed nullability checks on same shape#1378

Merged
jensneuse merged 10 commits intomasterfrom
fix/allow-differing-nullability-non-overlapping-types
Feb 16, 2026
Merged

feat: add flag for relaxed nullability checks on same shape#1378
jensneuse merged 10 commits intomasterfrom
fix/allow-differing-nullability-non-overlapping-types

Conversation

@jensneuse
Copy link
Copy Markdown
Member

@jensneuse jensneuse commented Feb 9, 2026

Summary by CodeRabbit

  • New Features

    • Optional relaxation of nullability validation for field-selection merging, controllable by configuration and validator options; factories/planners can opt in.
    • Per-instance pooling for planner resources to improve isolation and predictability.
  • Documentation

    • Added detailed rationale and guidance for the nullability-relaxation behavior.
  • Tests

    • Expanded validation, engine, datasource, planner, and resolve tests covering non-overlapping-type nullability scenarios.

jensneuse and others added 4 commits February 9, 2026 14:28
…e types

When inline fragments target different concrete object types (e.g. User vs
Organization), fields with the same name but different nullability (String!
vs String) are safe because only one branch ever resolves for a given object.
Previously the validator rejected these queries unconditionally.

Add TypesAreCompatibleIgnoringNullability to ast_type.go and use it as a
fallback when enclosing types cannot overlap (potentiallySameObject returns
false). Strict checking is preserved when an interface is involved or the
enclosing types are the same.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The nullability relaxation for field selection merging on non-overlapping
concrete types is now gated behind a feature flag instead of being always
active. This defaults to strict spec-compliant behavior.

Consumers opt in via:
- plan.Configuration.RelaxSubgraphOperationFieldSelectionMergingNullability
  (for upstream/subgraph operation validation)
- astvalidation.WithRelaxFieldSelectionMergingNullability()
  (for user-facing operation validation)

The plan package injects the flag into factories that implement the
SubgraphFieldSelectionMergingNullabilityRelaxer interface, keeping
the plan package decoupled from astvalidation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…union types

Thread validation options from plannerConfig through ExecutionEngine to
ValidateForSchema so the relaxed nullability flag is respected end-to-end.
Add three sub-tests: a negative test proving strict validation rejects
differing nullability, and two positive tests verifying correct resolution
for both non-null and null email values on different union member types.

Also change error comparison in runWithAndCompareError from Contains to
Equal for more explicit assertions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 9, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an optional, conservative relaxation for nullability checks during GraphQL field selection merging and threads that option through AST validators, planner/factory/printKit initialization, request validation, execution engine wiring, and tests for non-overlapping-type scenarios. Strict checks remain for potentially overlapping types.

Changes

Cohort / File(s) Summary
Type compatibility
v2/pkg/ast/ast_type.go, v2/pkg/ast/ast_type_test.go
Add TypesAreCompatibleIgnoringNullability and internal typesAreCompatible(..., ignoreNullability) to compare types while stripping NonNull wrappers; update TypesAreCompatibleDeep to use helper; add unit tests.
Validation API & caching
execution/graphql/validation.go
Request.ValidateForSchema now accepts variadic astvalidation.Option; DefaultOperationValidator(options...) used; per-schema validation cache bypassed when options are provided.
Operation validator options
v2/pkg/astvalidation/operation_validation.go
Add RelaxFieldSelectionMergingNullabilityCheck to OperationValidatorOptions and WithRelaxFieldSelectionMergingNullability(); pass flag into FieldSelectionMerging(...).
Field selection merging rule & docs
v2/pkg/astvalidation/operation_rule_field_selection_merging.go, v2/pkg/astvalidation/nullability_relaxation_justification.md
Make FieldSelectionMerging accept optional relaxNullabilityCheck; track enclosing type definitions; add potentiallySameObject() overlap check; conditionally use TypesAreCompatibleIgnoringNullability for non-overlapping concrete types; add justification doc.
Field selection tests
v2/pkg/astvalidation/operation_validation_test.go
Add tests covering differing nullability across non-overlapping object types and enforcement when interfaces could overlap; add schema snippets exercising scenarios.
Planner/Factory printKit pools
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go, v2/pkg/engine/datasource/graphql_datasource_test.go
Replace global printKit pool with per-Factory/per-Planner printKitPool(validationOptions...); add newPrintKitPool(...), relaxed pool initializer, EnableSubgraphFieldSelectionMergingNullabilityRelaxation() and wiring; add tests for inline fragments with differing nullability.
Test harness validation wiring
v2/pkg/engine/datasourcetesting/datasourcetesting.go, execution/engine/execution_engine_test.go
Add WithValidationOptions(...) test hook and validationOptions storage; expose test helper to enable relaxation in tests; tighten some error assertions.
Execution engine
execution/engine/execution_engine.go
Wire validation options into ExecutionEngine (compute/passthrough when config flag RelaxSubgraphOperationFieldSelectionMergingNullability is enabled) so validation uses configured options.
Configuration & planner creation
v2/pkg/engine/plan/configuration.go, v2/pkg/engine/plan/datasource_configuration.go
Add exported RelaxSubgraphOperationFieldSelectionMergingNullability flag to Configuration; introduce SubgraphFieldSelectionMergingNullabilityRelaxer interface and invoke factory hook when config flag is set during planner/datasource setup.
Resolve tests
v2/pkg/engine/resolve/resolve_test.go
Add tests ensuring resolver behavior respects per-type nullability for polymorphic fields across non-overlapping types.
Execution validation wiring
execution/engine/execution_engine.go, execution/engine/execution_engine_test.go
Compute and pass validation options into ValidateForSchema during execution when planner config or test flags enable nullability relaxation.

Sequence Diagram(s)

sequenceDiagram
  participant Config
  participant Factory
  participant Planner
  participant ExecutionEngine
  participant Request
  participant Validator

  rect rgba(200,200,255,0.5)
  Config->>Factory: set RelaxSubgraphOperationFieldSelectionMergingNullability
  Factory->>Factory: getPrintKitPool(validationOptions...)
  Factory->>Planner: create Planner(printKitPool)
  end

  rect rgba(200,255,200,0.5)
  ExecutionEngine->>Planner: obtain Planner (contains printKitPool)
  ExecutionEngine->>Request: ValidateForSchema(schema, validationOptions...)
  Request->>Validator: DefaultOperationValidator(options...)
  Validator->>Validator: FieldSelectionMerging(relaxFlag?)
  Validator-->>Request: validation result
  Request-->>ExecutionEngine: return result
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main objective: adding a flag to enable relaxed nullability checks for field selection merging on non-overlapping types with the same response shape.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into master

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/allow-differing-nullability-non-overlapping-types

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
execution/graphql/validation.go (1)

14-41: ⚠️ Potential issue | 🟠 Major

Cache key ignores validation options.

With options now affecting validation, caching only by schema hash can return a relaxed result for a strict call (or vice‑versa) when the same request is validated with different options. Consider bypassing the cache when options are provided, or incorporate an options hash into the key.

💡 Suggested fix (bypass cache when options are supplied)
 func (r *Request) ValidateForSchema(schema *Schema, options ...astvalidation.Option) (result ValidationResult, err error) {
 	if schema == nil {
 		return ValidationResult{Valid: false, Errors: nil}, ErrNilSchema
 	}
 
 	schemaHash := schema.Hash()
+	useCache := len(options) == 0
 
 	if r.validForSchema == nil {
 		r.validForSchema = map[uint64]ValidationResult{}
 	}
 
-	if result, ok := r.validForSchema[schemaHash]; ok {
-		return result, nil
-	}
+	if useCache {
+		if result, ok := r.validForSchema[schemaHash]; ok {
+			return result, nil
+		}
+	}
 
 	report := r.parseQueryOnce()
 	if report.HasErrors() {
 		return operationValidationResultFromReport(report)
 	}
 
 	validator := astvalidation.DefaultOperationValidator(options...)
 	validator.Validate(&r.document, &schema.document, &report)
 	result, err = operationValidationResultFromReport(report)
 	if err != nil {
 		return result, err
 	}
-	r.validForSchema[schemaHash] = result
+	if useCache {
+		r.validForSchema[schemaHash] = result
+	}
 	return result, err
 }
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)

1803-1808: Potential data race on lazy pool initialization.

getPrintKitPool() performs a non-atomic check-then-write on f.printKitPool. If Planner() is called concurrently when the pool is uninitialized, multiple pools could be created with one being lost.

If concurrent factory usage is expected, consider using sync.Once for initialization:

♻️ Thread-safe lazy initialization
 type Factory[T Configuration] struct {
 	executionContext   context.Context
 	httpClient         *http.Client
 	grpcClient         grpc.ClientConnInterface
 	grpcClientProvider func() grpc.ClientConnInterface
 	subscriptionClient GraphQLSubscriptionClient
 	printKitPool       *sync.Pool
+	printKitPoolOnce   sync.Once
+	relaxNullability   bool
 }
 func (f *Factory[T]) EnableSubgraphFieldSelectionMergingNullabilityRelaxation() {
-	f.printKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingNullability())
+	f.relaxNullability = true
 }

 func (f *Factory[T]) getPrintKitPool() *sync.Pool {
-	if f.printKitPool == nil {
-		f.printKitPool = newPrintKitPool()
-	}
+	f.printKitPoolOnce.Do(func() {
+		if f.relaxNullability {
+			f.printKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingNullability())
+		} else {
+			f.printKitPool = newPrintKitPool()
+		}
+	})
 	return f.printKitPool
 }
v2/pkg/astvalidation/operation_validation_test.go (1)

1080-1131: Add a relaxed‑mode overlap guard test.
The new cases cover non‑overlapping concrete types, but they don’t assert that FieldSelectionMerging(true) still rejects potentially overlapping types (e.g., interface vs object). Adding a variant will lock that behavior in.

🧪 Suggested test addition
+					t.Run("still rejects differing return type nullability when overlap is possible (relaxed)", func(t *testing.T) {
+						runWithDefinition(t, boxDefinition, `
+								{
+									someBox {
+										... on NonNullStringBox1 {
+											scalar
+										}
+										... on StringBox {
+											scalar
+										}
+									}
+								}`, FieldSelectionMerging(true), Invalid,
+							withValidationErrors(`fields 'scalar' conflict because they return conflicting types 'String!' and 'String'`))
+					})

devsergiy and others added 2 commits February 16, 2026 15:16
Use sync.Once to make the lazy initialization of printKitPool thread-safe
when multiple goroutines call Planner() concurrently.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go Outdated
Comment thread v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
Comment thread v2/pkg/engine/plan/configuration.go
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)

1723-1725: Avoid reconfiguring the pool after initialization.

EnableSubgraphFieldSelectionMergingNullabilityRelaxation() replaces the pool without synchronization; if called after any planner creation, you can end up with mixed validation behavior across planners and potential data races. Consider storing a flag and applying options only at first initialization.

♻️ Suggested refactor to make configuration one-time
 type Factory[T Configuration] struct {
 	executionContext   context.Context
 	httpClient         *http.Client
 	grpcClient         grpc.ClientConnInterface
 	grpcClientProvider func() grpc.ClientConnInterface
 	subscriptionClient GraphQLSubscriptionClient
 	printKitPool       *sync.Pool
 	printKitPoolOnce   sync.Once
+	relaxFieldSelectionMergingNullability bool
 }
 
 func (f *Factory[T]) EnableSubgraphFieldSelectionMergingNullabilityRelaxation() {
-	f.printKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingNullability())
+	f.relaxFieldSelectionMergingNullability = true
 }
 
 func (f *Factory[T]) getPrintKitPool() *sync.Pool {
 	f.printKitPoolOnce.Do(func() {
-		if f.printKitPool == nil {
-			f.printKitPool = newPrintKitPool()
-		}
+		opts := []astvalidation.Option{}
+		if f.relaxFieldSelectionMergingNullability {
+			opts = append(opts, astvalidation.WithRelaxFieldSelectionMergingNullability())
+		}
+		f.printKitPool = newPrintKitPool(opts...)
 	})
 	return f.printKitPool
 }

Also applies to: 1796-1811

- Store relaxation as bool flag, apply in sync.Once (devsergiy review)
- Bypass validation cache when custom options are provided (CodeRabbit review)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)

1797-1814: Consider adding a safeguard against out-of-order calls.

The sync.Once pattern means that if getPrintKitPool() (or Planner()) is called before EnableSubgraphFieldSelectionMergingNullabilityRelaxation(), the relaxation flag will be ignored silently. The comment documents the requirement, but a defensive check could prevent subtle bugs.

💡 Optional: Add a check or panic for out-of-order calls
 func (f *Factory[T]) EnableSubgraphFieldSelectionMergingNullabilityRelaxation() {
+	// Ensure this is called before the pool is initialized
+	if f.printKitPool != nil {
+		panic("EnableSubgraphFieldSelectionMergingNullabilityRelaxation must be called before Planner() or getPrintKitPool()")
+	}
 	f.relaxFieldMergingNullability = true
 }

Instead of per-factory pools, use two shared package-level pools
(default + relaxed). All factories/planners share the same pool,
avoiding redundant pool instances across datasources.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go`:
- Around line 1717-1721: The gci formatting linter flagged import
grouping/ordering that causes a failure near the var block (variables
defaultPrintKitPool, relaxedPrintKitPool, relaxedPrintKitPoolOnce); fix by
running your project's formatter (e.g., gci write) to auto-reorder import groups
and then run gofmt/goimports (or your repo's format command) to apply
formatting; re-run linters to confirm the import grouping issue is resolved and
commit the resulting changes.

Comment thread v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go`:
- Around line 1798-1806: The getKit and releaseKit methods on Planner[T] can
panic if p.printKitPool is nil; update getKit to use p.printKitPool if non-nil
otherwise fall back to defaultPrintKitPool when calling Get(), and update
releaseKit to reset and Put on p.printKitPool if non-nil otherwise use
defaultPrintKitPool; change the methods referenced are Planner.getKit,
Planner.releaseKit (kept used by printOperation and exposed via
DataSourcePlanner.ConfigureFetch/ConfigureSubscription) so the fallback to
defaultPrintKitPool prevents nil dereference when Planner is instantiated
directly.

Comment thread v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
Fall back to defaultPrintKitPool if printKitPool is nil, preventing
a panic if Planner is instantiated directly without Factory.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jensneuse jensneuse merged commit 6be2e74 into master Feb 16, 2026
10 checks passed
@jensneuse jensneuse deleted the fix/allow-differing-nullability-non-overlapping-types branch February 16, 2026 15:20
jensneuse pushed a commit that referenced this pull request Feb 16, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.8.0](execution/v1.7.0...execution/v1.8.0)
(2026-02-16)


### Features

* add flag for relaxed nullability checks on same shape
([#1378](#1378))
([6be2e74](6be2e74))


### Bug Fixes

* enable parallel execution for federation integration tests
([#1385](#1385))
([09d9348](09d9348))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **New Features**
  * Added a flag for relaxed nullability checks
* **Bug Fixes**
  * Enabled parallel execution for federation integration tests

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
jensneuse pushed a commit that referenced this pull request Feb 16, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.0.0-rc.250](v2.0.0-rc.249...v2.0.0-rc.250)
(2026-02-16)


### Features

* add flag for relaxed nullability checks on same shape
([#1378](#1378))
([6be2e74](6be2e74))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **New Features**
  * Added a new flag for relaxed nullability checks on shape validation.

* **Chores**
  * Updated version to 2.0.0-rc.250.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants